Add optional state-transition gas surcharge (CON-300) - #99

[suggestion] The helper targets 0x2, which is the SHA-256 precompile. In the tight-gas tests (TestGasSurchargeTraceReason, TestGasSurchargePassesTraceReason, TestGasSurchargeZeroIsUnchanged) the gas limit leaves exactly 0 gas after surcharge + intrinsic, so evm.Call fails the precompile's base cost, consumes everything, and sets result.Err = ErrOutOfGas. The UsedGas assertions then pass via the out-of-gas path rather than the intended successful call, and result.Err is never checked. Suggest a plain non-precompile recipient (e.g. 0x...beef) plus an explicit result.Err == nil assertion so these tests actually cover the success case. (Raised by the Codex pass; confirmed against vm.ActivePrecompiles.)

[suggestion] Prefer st.initialGas - st.gasSurcharge here (or st.gasRemaining + gas). The no-underflow guarantee for line 564 (st.initialGas - st.gasSurcharge - floorDataGas) comes from this admission check, but the check is written against msg.GasLimit while line 564 uses st.initialGas. They're equal only because initGas() sets initialGas = msg.GasLimit; sourcing both from initialGas makes the invariant local and keeps the pair correct if that ever changes.

[suggestion] Reusing ErrIntrinsicGas conflates "gas limit below the node-injected surcharge" with a genuine intrinsic-gas shortfall — callers (txpool, RPC error mapping) can't tell them apart, and the message text (want surcharge N) is the only signal. Consider a distinct sentinel (e.g. ErrGasSurcharge) wrapping/alongside it. Note this is a consensus-level rejection of the whole transaction, not a failed execution, which is why distinguishing it matters for admission control.

[nit] The core mechanism is generic (WithGasSurcharge takes a caller-supplied reason), so a Sei-domain-specific name in upstream-synced core/tracing is a bit incongruous — GasChangeTxSurcharge would describe what the gas change actually is, with "auto association" being the caller's use of it. Also flag for future upstream merges: value 20 is the next free slot upstream too, so a rebase that picks up a new upstream reason will collide here.

[nit] executionGasUsed underflows if it is ever reached before the surcharge has been deducted from gasRemaining. Both current call sites (refund cap, Prague floor) are post-deduction and safe — refunds are bounded by executionGasUsed/5, so the post-refund gasRemaining still cannot exceed initialGas - gasSurcharge — but a one-line comment stating that precondition would keep a future early-return from silently wrapping around.

[nit] The accessList fixture field is never passed to FloorDataGas (single-argument on this tree) — it only shows up in the failure message's entry count. Consider dropping the field and keeping just the access-list-ignored case's comment. Similarly, the pre-amsterdam/ case-name prefixes reference a fork that isn't on this tree, which reads as a stale upstream artifact.

[suggestion] GasChangeTxAutoAssociation is a Sei-specific reason but takes value 20, the next free slot in the upstream enum. When upstream geth adds its own reason at 20, a rebase will silently remap traces (and the hand-maintained gen_gas_change_reason_stringer.go ranges will need reshuffling again). Consider placing fork-specific reasons in a reserved high range (e.g. 200+, below GasChangeIgnored = 255) so upstream additions can never collide.

Also, the second line of the doc comment ("only when a surcharge is set") describes the caller's usage rather than the reason itself — WithGasSurcharge accepts any reason, so this constant carries no such guarantee.

[suggestion] Worth a doc comment stating the invariant, since a violation wraps silently rather than panicking. The subtraction is safe today because calcRefund caps the refund at executionGasUsed()/quotient, which keeps gasRemaining <= initialGas - gasSurcharge at both call sites (line 562 and line 672/675) — but that reasoning is non-local and easy to break with a future refund change. Something like:

// executionGasUsed returns the gas consumed by intrinsic cost and EVM execution,
// excluding the non-refundable surcharge. Safe from underflow because gasRemaining
// never exceeds initialGas-gasSurcharge after the surcharge is reserved.

[nit] msg.GasLimit - st.gasSurcharge recomputes a value that st.gasRemaining already holds: initGas sets gasRemaining = msg.GasLimit, the surcharge is subtracted at line 461, and intrinsic gas is not subtracted until line 493. Using st.gasRemaining directly removes the duplicated subtraction (and the need to re-derive the no-underflow argument from the line 454 guard).

[nit] Reusing ErrIntrinsicGas renders as intrinsic gas too low: have N, want surcharge M, which reads oddly and is indistinguishable from a genuine intrinsic-gas shortfall for callers matching on the sentinel. Reusing the class is a reasonable deliberate choice (it keeps the miner skipping the tx rather than treating the block as invalid), so if that's intentional a one-line comment saying so would help; otherwise a dedicated sentinel wrapping ErrIntrinsicGas would let callers tell them apart.

[nit] The surcharge OnGasChange fires before the intrinsic-gas check (line 476) and the floor-data check (line 486), so a tracer observes the surcharge deduction for transactions that are then rejected and never execute. Upstream's GasChangeTxInitialBalance in initGas has the same property, so this is consistent — but if you want the hook to only reflect executed transactions, moving the emission after the two admission checks (keeping the gasRemaining subtraction where it is) would do it.

[nit] This accessList field is never passed to FloorDataGas (which takes only data) — it is only echoed in the failure message at line 85. It's a vestige of the upstream Amsterdam rows that the PR description says were dropped. Same for the pre-amsterdam/ subtest name prefixes, which reference a fork that doesn't exist on this tree. Dropping the field and the prefixes would leave the intent clearer; if the "access-list-ignored" case is meant to document that the Prague floor is calldata-only, a comment alone conveys that.

[suggestion] msg.GasLimit - st.gasSurcharge recomputes a value st.gasRemaining already holds: initGas() sets gasRemaining = msg.GasLimit, and the surcharge was subtracted at line 461, so the two are equal here. Using st.gasRemaining directly keeps one source of truth, matches the intrinsic-gas check on line 476, and stays correct if initGas() ever stops setting gasRemaining == msg.GasLimit (at which point this expression could underflow, since the guard on line 454 is against gasRemaining, not msg.GasLimit).

[suggestion] Reusing ErrIntrinsicGas for a surcharge shortfall produces the self-contradictory message "intrinsic gas too low: have 999, want surcharge 1000", and makes surcharge failures indistinguishable from real intrinsic-gas failures for any errors.Is(err, ErrIntrinsicGas) consumer (txpool, RPC error mapping, metrics). A dedicated sentinel — e.g. ErrGasSurcharge in core/error.go, optionally wrapping ErrIntrinsicGas for backwards compatibility — would let callers tell the two apart.

[nit] This emits OnGasChange unconditionally on the caller-supplied reason, including when the caller passes tracing.GasChangeIgnored. Elsewhere (core/vm/contract.go:133,145) GasChangeIgnored means "suppress the hook, the change is emitted manually." Consider honoring that convention here for consistency: if reason != tracing.GasChangeIgnored.

[nit] The doc comment covers the accounting well but omits the usage contract a caller needs: it must be called before Execute(), it applies only to this top-level transition (ApplyMessage and nested calls are unaffected), and a gas limit below the surcharge surfaces as ErrIntrinsicGas. Worth stating, since the only caller lives outside this repo.

[nit] accessList is never passed to FloorDataGas — it only appears in the failure message on line 85 as len(tt.accessList). That makes the pre-amsterdam/access-list-ignored case (line 69) behaviourally identical to pre-amsterdam/non-zero-bytes-only: it can't fail for an access-list-related reason because the argument doesn't exist. Either drop the field, or keep it with an explicit comment that it is documentation-only. Separately, the pre-amsterdam/ name prefixes reference a fork that isn't on this tree, which reads as confusing here even though it eases future upstream diffing.

[suggestion] Reusing ErrIntrinsicGas for a surcharge shortfall makes the two failures indistinguishable to callers — RPC error mapping, txpool rejection reasons, and metrics all see "intrinsic gas too low" for a condition the sender cannot fix by looking at intrinsic cost. Consider a dedicated ErrGasSurcharge that wraps ErrIntrinsicGas (fmt.Errorf("%w: ...", ErrIntrinsicGas) already gives errors.Is compatibility, so a named sentinel costs nothing) so Sei's integration can report the actual cause.

[suggestion] This subtraction is unguarded. It is safe today — Execute always deducts the surcharge before calcRefund or the Prague floor branch can run, and the check at line 456 guarantees gasRemaining >= gasSurcharge — but if a future path ever reaches calcRefund without the deduction, this wraps to ~2^64 and the EIP-3529 refund cap silently disappears, which is a consensus-visible failure with no error. Either clamp (if st.gasUsed() < st.gasSurcharge { return 0 }) or state the invariant in the comment so the dependency on ordering is explicit.

[nit] Worth a short comment noting why this diverges from upstream's msg.GasLimit < floorDataGas: initGas sets gasRemaining = msg.GasLimit, so with no surcharge this is identical, and with a surcharge it deliberately checks the post-surcharge execution budget. As written the line looks like an unexplained deviation and will draw a question (or a bad resolution) on every upstream rebase of this file.

[nit] This test and TestGasSurchargeLeavesTooLittleForIntrinsic both assert only errors.Is(err, ErrIntrinsicGas), so neither actually proves which check fired — the surcharge test would still pass if the surcharge check were deleted and the intrinsic check caught it. TestGasSurchargeFloorAdmissionUsesExecutionBudget gets this right by asserting the exact message; do the same here (expect want surcharge 1000 vs want 21000).

[nit] This accessList field is never passed to anything — FloorDataGas takes only data, and the field is used solely in the failure message at line 85. That makes the access-list-ignored case a duplicate of non-zero-bytes-only, asserting nothing about access lists. Either drop the field and the case, or keep the case with a comment-only rationale.